[FEA] Support yyyyMMdd under CORRECTED policy [databricks] - #15820
[FEA] Support yyyyMMdd under CORRECTED policy [databricks]#15820gerashegalov wants to merge 5 commits into
Conversation
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
|
build |
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
|
build |
|
| "MM-yyyy", | ||
| "MM/dd/yyyy", | ||
| "MM-dd-yyyy", | ||
| "yyyyMMdd", |
There was a problem hiding this comment.
This list claims to be for both CORRECTED and EXCEPTION, and then there is a separate list for LEGACY, but I think that is misleading. EXCEPTION is supposed to:
- try the CORRECTED parser (returning on success)
- if it fails try the LEGACY parser
- if the LEGACY parser succeeds throw SparkUpgradeException
Per step 2 that means being supported under EXCEPTION also requires LEGACY to produce valid success/failure semantics. E.g. AI came up with the combination of 2024101 to yyyyMMdd under EXCEPTION, which should fail (Spark LEGACY accepts it) but we succeed (the JNI throws on it in LEGACY). I think we need three compatibility lists: CORRECTED, EXCEPTION, and LEGACY, where atm yyyyMMdd is not EXCEPTION compatible.
There was a problem hiding this comment.
Good catch. Fixed in 18bd197 by separating EXCEPTION_COMPATIBLE_FORMATS from CORRECTED_COMPATIBLE_FORMATS and selecting the policy-specific set during both tagging and execution. yyyyMMdd now falls back to CPU under EXCEPTION, so Spark preserves the CORRECTED/LEGACY disagreement behavior. I added coverage that asserts GetTimestamp fallback for a normal yyyyMMdd value and the expected error for 2024101. The focused Spark 3.5.7 GPU run passed.
There was a problem hiding this comment.
Looks good for yyyyMMdd. But we should check the other formats to see if anything else slipped by, with improper legacy semantics. I highly suspect many of the other formats are also wrong for EXCEPTION. Perhaps deferred to an issue + TODO comment.
There was a problem hiding this comment.
Audited and hardened in a880d83. EXCEPTION_COMPATIBLE_FORMATS is now an explicit allowlist rather than a subtraction from CORRECTED, and each of its 14 formats has a representative CORRECTED/LEGACY disagreement case exercised across five operators and ANSI on/off (140 cases). The matrix passed on Spark 3.5.7 and 4.0.1. The audit confirmed an additional mismatch for MMyyyy with input 12024, so that format now falls back under EXCEPTION. Issue #15977 and the adjacent TODO track restoring it after the fused parser preserves the disagreement.
| "MM-yyyy", | ||
| "MM/dd/yyyy", | ||
| "MM-dd-yyyy", | ||
| "yyyyMMdd", |
There was a problem hiding this comment.
On a separate note this list is also used by date_format, not just parsing. I don't know if we've verified that the reverse direction date_format(timestamp, 'yyyyMMdd') matches Spark.
There was a problem hiding this comment.
Verified and covered in 18bd197. yyyyMMdd is now included in the direct date_format parity matrix for both Date and Timestamp inputs, including the runtime-fallback and timezone-rule cases. The focused Spark 3.5.7 GPU run selected 25 yyyyMMdd cases: 24 passed and the Spark-4-only case skipped, with no failures or errors.
There was a problem hiding this comment.
For the signed/extended years we are not matching Spark:
SELECT date_format(
timestamp_micros(327403411200000000L + id),
'yyyyMMdd')
FROM range(1)CPU: +123450101, GPU: 23450101.
There was a problem hiding this comment.
Fixed in a880d83. Formatting now has its own compatibility direction, and any certified format whose converted pattern contains %Y falls back when spark.rapids.sql.hasExtendedYearValues=true, unless the existing incompatible-date-formats opt-in is enabled. I added extended-year fallback coverage for all 12 certified year-bearing formatting patterns plus GPU parity checks for the four patterns without a year. The focused suites passed on Spark 3.5.7 (22 tests) and Spark 4.0.1 (23 tests).
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
|
build |
1 similar comment
|
build |
| // The fused parser only accepts an unsigned four-digit year for this packed format. | ||
| if (expr.left.dataType == DataTypes.StringType && sparkFormat == "yyyyMMdd") { | ||
| YearParseUtil.tagParseStringAsDate(conf, this) | ||
| } |
There was a problem hiding this comment.
Before this change LEGACY + spark.rapids.sql.incompatibleDateFormats.enabled=true accelerated yyyyMMdd without also requiring spark.rapids.sql.hasExtendedYearValues=false. I think this guard is overbroad
There was a problem hiding this comment.
Fixed in a880d83. The packed yyyyMMdd extended-year parsing guard now applies only under CORRECTED. LEGACY with spark.rapids.sql.incompatibleDateFormats.enabled=true retains the existing acceleration behavior even when spark.rapids.sql.hasExtendedYearValues=true; a regression test asserts CPU/GPU parity and GPU execution for that configuration on Spark 3.5.7 and 4.0.1.
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Fixes #15588.
Follow-up issue: #15977
Description
The
yyyyMMddformat currently falls back to CPU under the CORRECTED time parser policy unless incompatible date formats are enabled. The customer workload described in #15588 was 14.9% faster when the existing GPU path was forced.This change:
yyyyMMddfor the fused JNI parser under CORRECTED whenspark.rapids.sql.hasExtendedYearValues=false;yyyyMMddon CPU under EXCEPTION so Spark can detect CORRECTED/LEGACY disagreements such as2024101;MMyyyyon CPU under EXCEPTION after confirming that input12024throws on Spark CPU but returns null in the fused GPU parser;Issue #15977 tracks restoring
MMyyyyEXCEPTION acceleration after the fused parser preserves parser-policy disagreements.Local verification:
mvn -T64 -pl sql-plugin -am -DskipTests verifymvn -T64 package -pl dist,integration_tests -am -DskipTests -Dbuildver=357mvn -T64 -f scala2.13/pom.xml package -pl dist,integration_tests -am -DskipTests -Dbuildver=401git diff --checkChecklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance